UCP/CORE: Fix memory cache lookup return value handling #7737

dmitrygx · 2021-12-01T16:23:39Z

What

Fix memory cache lookup return value handling.

Why ?

The return value could be either UCS_OK or UCS_ERR_NO_ELEM.

How ?

Re-organize the code of branches.
Add useful assertions.
Remove incorrect assertion.
Remove extra brackets around a condition.

yosefe · 2021-12-01T16:40:31Z

src/ucp/core/ucp_context.h

-        }
+    if (ucs_likely(status == UCS_ERR_NO_ELEM)) {
+        goto out_host_mem;
+    } else {


remove else?

fixed and force-pushed (the PR is small enough)

yosefe · 2021-12-01T16:41:52Z

@dmitrygx the problem was extra detect overhead when not needed?

yosefe · 2021-12-01T16:54:54Z

src/ucp/core/ucp_context.h

+
+    if ((mem_info->type != UCS_MEMORY_TYPE_UNKNOWN) &&
+        (mem_info->sys_dev != UCS_SYS_DEVICE_ID_UNKNOWN)) {
+        return;


maybe move line 517 inside this if and inverse it's condition

maybe remove this return?

yosefe · 2021-12-01T16:55:05Z

src/ucp/core/ucp_context.h

+     * memory domains. In any case, the memory type cache is not expected to
+     * return HOST memory type.
+     */
+    ucs_assert(mem_info->type != UCS_MEMORY_TYPE_HOST);


maybe move this assert to line 505

good catch, done

dmitrygx · 2021-12-01T17:24:26Z

@dmitrygx the problem was extra detect overhead when not needed?

yes, if it returns UCS_ERR_NO_ELEM, it is always HOST memory - since we don't add HOST memory buffers to memtype cache.
it moves the behavior to what we have introduced in a29b2f7 (PR #6180)

also, it fixes incorrect assertion:

if (status != UCS_ERR_NO_ELEM) {
        if (ucs_likely(status != UCS_OK)) {
            ucs_assert(status == UCS_ERR_NO_ELEM); // <---- this one, it was checked above
            goto out_host_mem;
        }
...
}

yosefe · 2021-12-04T21:35:32Z

src/ucp/core/ucp_context.h

+
+    if ((mem_info->type != UCS_MEMORY_TYPE_UNKNOWN) &&
+        (mem_info->sys_dev != UCS_SYS_DEVICE_ID_UNKNOWN)) {
+        return;


maybe remove this return?

yosefe · 2021-12-04T21:35:42Z

src/ucp/core/ucp_context.h

+        goto out_host_mem;
+    }
+
+    ucs_assertv((status == UCS_OK) || (status == UCS_ERR_UNSUPPORTED),


why would we get UCS_ERR_UNSUPPORTED?

I've updated PR to return UCS_ERR_UNSUPPORTED in case of MEMTYPE is disabled to distinguish between no element (HOST mem buffer) or always use memory detection (slowpath)

what is the advantage? it adds another branch (line 507)

what is the advantage? it adds another branch (line 507)

it fixes the problem with memory type detection if MEMTYPE cache wasn't enabled - we should go to memory detection through MDs. without this check - we assumed that memory type is HOST.

interesting...
maybe we should set mem_info->type=UNKNOWN in this case?

Actually it would be weird to set mem_info in case of error return,
so guess better restore an equivalent of the prev logic where we checked if memtype_cache was enabled in the first place. keep it UNSUPPORTED

so guess better restore an equivalent of the prev logic where we checked if memtype_cache was enabled in the first place. keep it UNSUPPORTED

do you mean checking status == UCS_ERR_UNSUPPORTED and doing memory detection in this case?

yosefe · 2021-12-05T16:38:16Z

src/ucp/core/ucp_context.h

-    if ((status == UCS_ERR_UNSUPPORTED) ||
-        (mem_info->type == UCS_MEMORY_TYPE_UNKNOWN) ||
-        (mem_info->sys_dev == UCS_SYS_DEVICE_ID_UNKNOWN)) {
+    if (ucs_unlikely((status == UCS_ERR_UNSUPPORTED) ||


pls simplify, i think we can avoid "goto" now

host memory is the fast path and should be checked first

host memory is the fast path and should be checked first

done

pls simplify, i think we can avoid "goto" now

can't remove goto - two places use it - and can't merge them into single if statement, because context->num_mem_type_detect_mds == 0 must be checked before calling ucs_memtype_cache_lookup()

yosefe reviewed Dec 1, 2021

View reviewed changes

yosefe requested a review from Akshay-Venkatesh December 1, 2021 16:42

dmitrygx force-pushed the topic/ucp/mem_detect_fix branch from 8610669 to 2e90f60 Compare December 1, 2021 16:53

yosefe reviewed Dec 1, 2021

View reviewed changes

brminich approved these changes Dec 3, 2021

View reviewed changes

yosefe reviewed Dec 5, 2021

View reviewed changes

yosefe approved these changes Dec 5, 2021

View reviewed changes

UCP/CORE: Fix memory cache lookup retrun value handling

8659677

dmitrygx force-pushed the topic/ucp/mem_detect_fix branch from bd42ac7 to 8659677 Compare December 5, 2021 16:59

dmitrygx merged commit 3818f46 into openucx:master Dec 5, 2021

yosefe mentioned this pull request Feb 5, 2022

Memtype cache fixes - v1.12.x #7909

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UCP/CORE: Fix memory cache lookup return value handling #7737

UCP/CORE: Fix memory cache lookup return value handling #7737

dmitrygx commented Dec 1, 2021

yosefe Dec 1, 2021

dmitrygx Dec 1, 2021

yosefe commented Dec 1, 2021

yosefe Dec 1, 2021

dmitrygx Dec 1, 2021

yosefe Dec 4, 2021

dmitrygx Dec 5, 2021

yosefe Dec 1, 2021

dmitrygx Dec 1, 2021

dmitrygx commented Dec 1, 2021 •

edited

Loading

yosefe Dec 4, 2021

yosefe Dec 4, 2021

dmitrygx Dec 5, 2021

yosefe Dec 5, 2021

dmitrygx Dec 5, 2021

yosefe Dec 5, 2021

yosefe Dec 5, 2021 •

edited

Loading

dmitrygx Dec 5, 2021

yosefe Dec 5, 2021

dmitrygx Dec 5, 2021

yosefe Dec 5, 2021

dmitrygx Dec 5, 2021

UCP/CORE: Fix memory cache lookup return value handling #7737

UCP/CORE: Fix memory cache lookup return value handling #7737

Conversation

dmitrygx commented Dec 1, 2021

What

Why ?

How ?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yosefe commented Dec 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitrygx commented Dec 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yosefe Dec 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitrygx commented Dec 1, 2021 •

edited

Loading

yosefe Dec 5, 2021 •

edited

Loading